Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V5 trade protocol #7105

Draft
wants to merge 52 commits into
base: master
Choose a base branch
from
Draft

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented May 17, 2024

Preliminary changes on top of the the work done by @alvasw and @HenrikJannsen on the new v5 trade protocol upgrade for bisq1 (see bisq-network/proposals#421), recently rebased to master (3b428df - May 9, 2024).

The changes attempt to avoid an increase in the number of message rounds at the start of the protocol, by having both traders compute all the finalised staged txs excluding the claim txs (that is, both warning txs and both redirect txs) at the earliest available opportunity. I believe this is upon receipt of the first response from the maker (InputsForDepositTxResponse_v5), if enough information is provided in the first two messages, namely the fee bump output addresses and the signatures of the staged txs once the deposit txId is known. Only exchange of the staged tx signatures is necessary, rather than the txs themselves, as a single signature commits to the entire tx (minus witnesses), provided the tx is non-malleable.

For the buyer-as-taker, seller-as-maker protocol, the old DepositTxMessage from v4 is redundant in the case of SegWit-only inputs. (I think it would make sense to completely disallow non-SegWit deposit tx inputs for v5, which should probably be done for v4 as well if they're still allowed, as it gives better safely against an invalid delayed payout tx due to deposit tx malleability.) Since that message is redundant, it should be possible to achieve one fewer message rounds in that case, so that there is one less message exchanged when the buyer is the taker, instead of there being one more message exchanged, at present.

TODO: So far, I have wired up the protobuf messages but not finished adding the finalised staged tx computations to the TradeTask lists.

TODO: Organise the bundled changes into a more logical sequence of commits.

TODO: Rest of the protocol, beyond the trade start.

@djing-chan
Copy link
Contributor

Thanks for sharing and great to see that progress!

For the buyer-as-taker, seller-as-maker protocol, the old DepositTxMessage from v4 is redundant in the case of SegWit-only inputs. (I think it would make sense to completely disallow non-SegWit deposit tx inputs for v5, ...

Agree

...which should probably be done for v4 as well if they're still allowed, as it gives better safely against an invalid delayed payout tx due to deposit tx malleability.)

Agree

Since that message is redundant, it should be possible to achieve one fewer message rounds in that case, so that there is one less message exchanged when the buyer is the taker, instead of there being one more message exchanged, at present.

I guess you refer to v4 protocol, right? Do you think the added value of reducing one round justifies the risks by changing the protocol? Specially in the light that once v5 is complete v4 will get faded out anyway (we can enforce it after some period).

@stejbac
Copy link
Contributor Author

stejbac commented May 19, 2024

I wasn't planning to make any changes to the v4 protocol, except possibly disallowing non-Segwit inputs if they still work. But I was aiming to reduce the number of rounds for v5, in the case of the buyer-as-taker. Hopefully, I won't run into problems when adding the missing TradeTasks.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@HenrikJannsen
Copy link
Collaborator

Still active (-> avoid stale bot close)

@stejbac
Copy link
Contributor Author

stejbac commented Jul 2, 2024

I've force pushed a rebase from the latest snapshot of master, and added some further temp changes. The trade task-lists at the opening of the trade are mostly done, but need testing. The redirect tx finalisation logic also needs fixing. Then, work on the rest of the trade lifecycle can be started.

Currently, it is set up so that both traders end up with each others' finalised warning & redirect txs, in addition to their own. I'm not sure whether this poses any security issue, but should be fairly easy to change if necessary. So one could publish the peer's redirect tx after publishing one's own warning tx, to send the trade straight to arbitration. Or, less usefully, publish the peer's warning tx.

@stejbac
Copy link
Contributor Author

stejbac commented Jul 6, 2024

I've pushed another commit to fix the warning/redirect/claim txs, so that they are publishable and mineable on regtest. I've also pushed a commit to enable the trade to open successfully (for both maker/taker roles of the buyer/seller), so that it may proceed without error to the trade close, at least in the happy path (excluding minor UI errors relating to the missing delayed payout tx).

Next, I intend to tidy up my commits, fix the UI issues, then work on the paths where the warning/redirect/claim txs get broadcast.

@pazza83
Copy link

pazza83 commented Jul 19, 2024

@stejbac

Asked this question on the Single Transaction Multisig Protocol for Bisq using UTXO swap

Thought it would be a good idea to ask here also:

Alice is half way through a trade with Bob when she suffers a hard drive crash. Bob having not received payment from Alice publishes the Warning Tx. What does Alice require to publish her Redirect Tx?

Just wondering if she needs something other than just her seed phrase?

@stejbac
Copy link
Contributor Author

stejbac commented Jul 20, 2024

Yes, I did think about that before a bit and there is definitely a problem if a trader suffers data loss half way through a trade, as it won't be possible to reconstruct the redirect tx just from your own seed phrase. (Also, I suppose a trader who didn't set out to scam his peer might nevertheless be tempted to use the claim tx in such a scenario, so there could be a number of such cases.)

Any single-tx protocol will similarly have much more severe problems in case of data loss, at least upon trade closure until the payout output is spent, and would ideally use a robust backup solution for critical data -- perhaps some kind of cloud backup using the P2P network (with data encrypted with key material derived from the seed phrase). It's a similar problem that lightning faces with the need for channel backups, or anything that moves activity off chain.

There is another potential, albeit slightly hacky solution in this case:

By manipulating signature nonces, it's possible to hide a small amount of private data in the signature that can be recovered from the seed phrase. Each signature is (morally) 512 bits and can hide up to 256 bits in this way. In this case, the peer's signature for the redirect tx is the critical data that needs backing up. So that 512 bits can be split in half, with one half hidden in your own warning tx signature and the other half hidden in the signature for your own input to the deposit tx. One has to be very careful playing about with nonces in this way, as messing it up can leak private keys, but I'm pretty confident it can be done in a way that's secure. Such an enhancement wouldn't increase the number of message rounds at the trade start, as far as I can tell, and probably wouldn't be all that disruptive, but would involve a smallish amount of low level elliptic curve logic to provide an unsafe signing API with custom nonces.

@pazza83
Copy link

pazza83 commented Jul 20, 2024

Thanks for the answer.

I suppose the trader with data loss not being able to reconstruct the redirect tx just the seed phrase alone is not too big of a difference from the current trade protcol. Currently a trader in mediation with data loss is unable to publish the arbitration transaction with the seed phrase alone (they need a copy of the DPT).

perhaps some kind of cloud backup using the P2P network (with data encrypted with key material derived from the seed phrase).

Yes some sort of backup would be good. A p2p solution was created here #6589 but did not end up being used.

What about the case where there is data loss but the claim transaction has not been published. Would both buyer and seller be able to make a manual payout using the seed pharse alone (assuming they could contact eachother)?

@stejbac
Copy link
Contributor Author

stejbac commented Jul 20, 2024

@pazza83 Yes, just like in the v4 protocol, it will always be possible for two cooperating traders to make a manual payout just from their respective seed phrases, since then both the buyer and seller multisig private keys can be recovered, which is all that's needed to spend the deposit or warning tx escrow output.

@stejbac
Copy link
Contributor Author

stejbac commented Jul 20, 2024

Regarding #6589, I think the backup storage requirements (which are worse now with the much larger multi-output DPTs) could be eased somewhat by only storing the buyer & sellers signatures for the delayed-payout/warning/redirect txs, rather than the entire tx, as all those txs can be reconstructed (without signatures) just by knowing the DAO block height agreed at the start of the trade (guessing and working backwards from the deposit tx block height), the buyer & seller public keys (recoverable from the respective seed phrases), the agreed deposit tx fee, and of course the deposit tx outpoint itself.

(In fact, one could probably shave off a few bytes from the signatures themselves, including all the metadata like the sighash flags and the DER length/type encodings, to get a bit less than 128 bytes per signature pair, plus the backup encryption IV, by just brute-forcing the missing bytes.)

(Thinking about it further, if you just stored the XOR of the two signatures, you could halve the size to 64 bytes per signature pair needed for each staged tx, as each signature is a deterministic function of the private key and the data being signed, and one of the signatures is thus always recoverable, allowing the other to be recovered from the XOR.)

@stejbac
Copy link
Contributor Author

stejbac commented Jul 20, 2024

Edit to the above:

I missed that the peer's multisig public key isn't recoverable if all you have is the deposit tx on-chain and your own seed phrase, so that would have to be backed up as well, XORed with your own multisig public key for an additional 32 bytes per trade.

@stejbac
Copy link
Contributor Author

stejbac commented Jul 24, 2024

I rebased the branch again to resolve conflicts. I also pushed a few more commits to further improve the redirect tx fee calculation, and fix all the UI & log errors/warnings I spotted in the happy path of the trade protocol.

Next, I plan to work on the publishing of the warning, redirect and claim txs, and make sure the peer will always pick them up, regardless of receipt of a trade message, which I think will require adding suitable watched scripts to the SPV wallet. (The potential race between the staged txs could complicate things, as bitcoinj doesn't handle replacement well - I'm not sure.)

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@HenrikJannsen
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Still active

@stejbac
Copy link
Contributor Author

stejbac commented Sep 6, 2024

I've pushed a couple more commits (add some missing @Nullable annotations & fix/optimise Transactions view) and rebased from master.

alvasw and others added 4 commits September 13, 2024 12:59
The factory can create, sign, and finalize the warning transaction.
The factory can create, sign, and finalize the redirection transaction.
The factory creates, signs, and finalizes the claim transaction.
…ocol to BaseSellerProtocol

Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
HenrikJannsen and others added 23 commits September 13, 2024 12:59
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
TODO: check if that is expected when we do not have the full tx chain or if its caused by a bug.

Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
Also add trade tasks to publish the warning/redirect/claim txs.
Also make claim delay 5 blocks on regtest, and add missing TradeMessage
constructors to provide a default argument for the P2P message version,
for consistency with the other trade messages.
Add an extra flag to 'DelayedPayoutTxReceiverService.getReceivers(..)'
to control the tx fee precision, and refactor to use an EnumSet of flags
activated by particular date, instead of just passing boolean args. This
new 'PRECISE_FEES' flag is currently set to activate at the same time as
the v5 protocol upgrade (though it can be used independently).

When present, the flag changes the receiver list calculation as follows:

 1) Spendable amount depends on individual output ScriptPubKey sizes,
    instead of all outputs assumed to cost 32 bytes each (P2SH cost);
 2) Base cost of DPT is that of the signed instead of unsigned tx (v4
    protocol only - redirect tx base cost is always for the signed tx);
 3) Increase in spendable amount from saved tx fees after filtering out
    the small outputs is taken into account;
 4) Small outputs are filtered out pre-adjustment upwards, rather than
    post-adjustment, so that they don't get erroneously included;
 5) The balance given to the LBM takes the tx fee cost of his output
    into account.

Additionally, restrict the fee bump addresses of the peer's warning and
redirect txs to be P2WPKH, for more predictable tx fee rates.
Make sure the 'signedClaimTx' & 'finalized(Warning|Redirect)Tx' fields
of the 'ProcessModel' & 'TradingPeer' models are persisted properly in
the respective proto objects. To this end, store them serialised as byte
arrays instead of Transaction objects.

(Also clean up the Lombok annotations slightly.)
Add checks that we're not running the v5 protocol, everywhere a missing
delayed payout tx would cause errors or warnings to appear in either the
logs or the Pending Trades or Trade Details views, for a v5 trade that
completes normally.

Also add both the buyer's & seller's redirect & warning txs to the Trade
Details view, in place of the missing DPT, as well as the claim tx if
it's present. (The latter is created & signed at the point of use.) Add
suitable 'get*(BtcWalletService)' methods to 'Trade' for that purpose.
Make 'aesKey' method parameters @nullable where obviously applicable,
for consistency (as it was missed out in a number of places). Also make
the v5-protocol-specific fee bump fields of 'InputsForDepositTxRequest'
@nullable, as expected.
Make sure 'TransactionAwareTrade::isRelatedToTransaction' returns true
for warning, redirect & claim txs belonging to the given trade. Also
optimise the method somewhat by short circuiting on a wider class of txs
than those with zero locktime, when ruling out that the tx is a delayed
payout or warning tx. The previous short circuit test was inadequate due
to the fact that a lot of wallets, such as Sparrow, set a nonzero
locktime on all txs by default, to prevent fee sniping.

Also modify 'TransactionAwareTradable::bucketIndex' to place the new
staged txs in the (global) delayed payout tx bucket, so that they get
past the related transactions filter, used to speed up the pairing of
txs with tradables.
Since the multisig escrow outputs of the deposit & warning txs do not
belong to the user, bitcoinj won't pick up any txs spending them unless
a corresponding watched script (the ScriptPubKey) is added to the
wallet. To this end, provide a trade task to add watched scripts for
those three outputs, which runs just before the client or the peer might
broadcast the deposit tx. Also remove them upon withdrawal of funds at
the end of the trade (closed normally or through a dispute).

We need to add watched scripts for the deposit tx output and both the
user's and the peer's warning tx outputs, so that the peer's warning,
redirect and claim txs are all picked up, regardless of any message sent
to the client.

TODO: Possibly find a way to clear out old watched scripts from failed
 trades, as they will otherwise remain in the user's wallet permanently,
 creating a growing burden for the wallet. Also, we should possibly re-
 add all the watched scripts if the wallet is restored from seed.
Ensure 'TransactionsListItem' recognises warning, redirect & claim txs
and displays appropriate details messages for them. Redirect txs are
made to show the same "Refund collateral" details message as delayed
payout txs and don't distinguish between the user's or peer's tx,
whereas warning & claim tx details do distinguish between them.

Also ensure the correct amounts are displayed in the Transaction view,
when watched scripts are present in the wallet, by changing
'WalletService::getValueSent(To|From)MeForTransaction' not to include
watched outputs or inputs in their respective sums.

Ensure claim txs broadcast by the peer are correctly linked to the trade
and display correctly in the Transactions view, by changing
'BisqRiskAnalysis' not to deem txs with a relative lock time as risky,
as that interferes with the v5 trade protocol.

Finally, make the Trade Details window resilient to missing peer's
redirect & warning tx from old trades, which could be cleared out as
sensitive data, and prevent it from incorrectly displaying the claim tx
as the multisig payout tx (and similarly for the Transactions view).
Also update the expected signed tx sizes accordingly, and require that
the peer provides a low-R signature for them, so that they're never
bigger than expected. (No such requirement is made of any of the txs in
the current v4 protocol, to ensure backwards compatibility.)
Add the 4 values 'WARNING_SENT(_BY_PEER)' & 'ESCROW_CLAIMED(_BY_PEER)'
(all unused at present) to the 'Trade.DisputeState' enum, and update the
proto. The new states are not classed as arbitrated, as arbitration is
only deemed to occur once a redirect tx has been published (and that's
intended to reuse the existing 'REFUND_REQUEST*' dispute states of the
current v4 protocol). But they are an escalation beyond mediation, as
they spend the escrow, and thus disable both the buyer and seller
payment confirmation. Accordingly, add a 'DisputeState::isEscalated'
predicate to include the new states in addition to the arbitrated ones.
Adapt the existing workflow of starting a second-round arbitration
process, upon mediation failure, to the v5 trade protocol, by giving the
trader an option to broadcast his warning tx. This replaces the current
(tertiary) action of broadcasting the (v4 protocol) delayed payout tx to
start arbitration, on the mediation result popup. Instead, the trader
must now wait for the peer to see the warning tx and actually start
arbitration by broadcasting his redirect tx. (This second part is not
yet implemented.)

Also clean up 'DisputeValidation' slightly and prevent the errant
display of a duplicate-DPT-detected message in the event that a dispute
has a missing delayed payout txId (as is currently the case for v5
protocol trades). Fix the logic similarly for missing trade IDs &
deposit txIds.

TODO: Allow peer to start arbitration by broadcasting his redirect tx,
 upon detection (via a suitable listener) of a warning tx broadcast.
@stejbac
Copy link
Contributor Author

stejbac commented Sep 15, 2024

I've pushed a few more commits, which add extra dispute states and allow a trader to publish his warning tx at the end of mediation, in place of the DPT broadcast of the v4 protocol, and rebased from master. I'm currently working on adding a listener to pick up the warning tx broadcast and enable a trader to actually start arbitration, by publishing the redirect tx in response.

Provide a 'SetupWarningTxListener' trade task, which runs at the opening
of the trade and upon initialisation of the trade manager at application
startup. It adds a listener which picks up either warning tx and updates
the dispute state to 'WARNING_SENT(_BY_PEER)', as appropriate.

As the peer's warning tx may be unknown (at least in the unlikely event
that sensitive data was cleared out of an unfailed trade), the listener
detects any spend of the deposit tx escrow output. (This functionality
will also be needed to pick up the peer's claim tx, which has a
completely unknown txId.) To this end, provide a new listener type,
'OutputSpendConfidenceListener', which can be added to or removed from a
'WalletService' instance and detects change in the confidence of any tx
spending the provided (non-detached) 'TranactionOutput' instance.

(Also do some minor cleanup of the 'WalletService' class.)
When either of the trade peers publishes his warning tx, reflect that in
the info panel of the trade step view, providing a red "Redirect to
arbitration" or a (possibly greyed out) green "Claim trade collateral"
button, in place of the usual get-help/open-dispute button. Add four new
values to the 'TradeStepInfo.State' enum to distiguish whose warning tx
was published and whether the corresponding claim tx is still locked.
Provide (currently unimplemented) button action stubs to open a popup to
claim/redirect.

Also do some minor cleanup of 'TradeStepView' and make sure the method
'DisputeManager::checkForMediatedTradePayout' closes the mediation
ticket upon publishing of either warning tx, not just upon starting
arbitration or receiving a payout.
Rename 'SetupWarningTxListener' to 'SetupStagedTxListeners' and add code
to provide a second listener for the redirect or claim tx, upon firing
of the first listener. Set the trade dispute state to one of the four
states 'REFUND_REQUEST(_STARTED_BY_PEER)' or 'ESCROW_CLAIMED(_BY_PEER)',
as appropriate, upon firing of the downstream listener. Also, restore
the peer's redirect or warning tx in the unlikely event that they were
cleared out as sensitive data, and fill in the peer's claim tx if it
gets picked up. Add a proto field to persist the latter, in order to
show it in the details window of a past trade.

Make sure that the peer's staged txs don't get subsequently removed as
sensitive data if the trade wound up in a dispute and any staged txs
were broadcast. Similarly, suppress the removal of watched scripts in
that case, to prevent staged txs disappearing if there's an SPV resync.

Finally, add a missing 'SetupStagedTxListeners' trade task item to the
'PreparedTxBuyerSignaturesMessage' handler of the v5 seller-as-maker
protocol (overriding super), as the listeners weren't being set up at
the start of the trade in that case.
Make the filtering methods 'isPossible(RedirectOrClaimTx|EscrowSpend)',
used to speed up the Transactions view, lenient towards segwit txs that
have missing witness data. This appears to be the case for a lot of txs
fetched from the network by bitcoinj, including all the segwit txs in
the wallet after an SPV resync.

Since the witness data of a peer's claim tx (picked up by bitcoinj) may
in fact be missing, rename the field 'TradingPeer.signedClaimTx' to
'claimTx', along with corresponding proto field.

Finally, make sure the correct details message is shown for refund agent
payout txs, for v5 protocol trades in the Transactions view, by changing
the order of some of the nested if-else branches, in order to avoid an
unwanted '!trade.hasV5Protocol()' clause.
Implement the redirect-to-arbitration & claim buttons in the trade step
view, allowing the warned peer to publish the redirect tx and open a
refund dispute, or the user to close the trade by publishing the claim
tx if an unresponsive peer. To this end, add '(warning|redirect)TxId'
fields to the 'Dispute' DTO and corresponding proto, to use in place of
the (now null) 'delayedPayoutTxId' field. The new fields allow the
refund agent to validate the tx chain in the case of the v5 protocol
(using the Mempool service to make sure the redirect tx is valid and
published/confirmed -- not yet implemented) and guard against replay
attacks. Update the validation logic accordingly (& add some TODOs).

Additionally, tidy up 'DisputeSummaryWindow' a little and fix a bug in
'SetupStagedTxListeners' preventing the redirect/claim tx listener from
firing after the warning tx appears, until an application restart. Also
ensure that it closes the trade instead of merely updating the dispute
state, when a claim tx is picked up.

TODO: Improve popup messages & fix arbtrator tx chain validation logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants